[Hub Generated] Review request for Microsoft.ContainerRegistry to add version preview/2019-12-01-preview#7883
[Hub Generated] Review request for Microsoft.ContainerRegistry to add version preview/2019-12-01-preview#7883raych1 merged 11 commits intoAzure:masterfrom jikuma:dev-containerregistry-Microsoft.ContainerRegistry-2019-12-01-preview
Conversation
…e/2019-05-01 to version 2019-12-01-preview
Automation for azure-sdk-for-pythonA PR has been created for you: |
Automation for azure-sdk-for-goNothing to generate for azure-sdk-for-go |
There was a problem hiding this comment.
How about follow the yyyy-MM-preview pattern for the tag?
There was a problem hiding this comment.
I do not remember changing this file, this file was changed by "OpenApi Hub" on behalf of me.
There was a problem hiding this comment.
Thanks for the info. We will log a bug for OpenApi Hub.
|
@qiaozha , can you please take a look at the JS SDK failures? |
|
@jikuma This problem occurs because in your readme.md you've already set your default tag
|
There was a problem hiding this comment.
For all API responses in this spec, please add a "default" response to capture error cases. It should have a schema consistent with the common error contract. Here is an example:
Even if this did not exist in prior versions, let's get it added going forward
There was a problem hiding this comment.
The swagger file is auto-generated. I do not think we should edit this file. I think we are missing some decorator on the controller for this. Can you point me to an example?
Secondly, I do not want to make changes to the old controller/api as part of this PR. I will open WI to track these changes.
There was a problem hiding this comment.
Why is this "listUsages" and not /usages? It should follow the contract.
There was a problem hiding this comment.
I do not want to make changes to the old controller/api as part of this PR. I will open WI to track these changes.
There was a problem hiding this comment.
Can you help me understand why a new version (a preview too) is not a good opportunity to correct issues? I'm not sure what you mean by "old controller/api"
There was a problem hiding this comment.
changes the api and made it usages from listUsages
There was a problem hiding this comment.
@KrisBash I see that there is a contract to return the resource usages across Azure. But this API is not exactly the contract implementation as the payload is different and back then we didn't have this contract. Also, it loses its ability to call from ARM template if we remove list prefix. We need to evaluate our API usage and then introduce this change. Can we please take this change as a separate item and scope this version to customer managed key scenarios.
There was a problem hiding this comment.
There are a number of nested resource types from the prior version not carried forward. Is that intentional? It's a suboptimal experience for the customer to produce a template to configure a resource if they have to use different API versions to configure different element/sub-resources.
There was a problem hiding this comment.
Added the deleted controllers back due to consistency.
|
/openapi sdkautomation |
azure-sdk-for-go - Release
|
azure-sdk-for-java - Release
|
azure-sdk-for-net - Release
|
azure-sdk-for-python - Release
|
azure-sdk-for-js - Release
|
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Can one of the admins verify this patch? |
KrisBash
left a comment
There was a problem hiding this comment.
Discussed offline. Two issues must be fixed in a future update:
- "default" response for all APIs
- listUsages normalization to standard /usages contract
|
@ArcturusZhang , can you have a look at the SDK Go failures and suggest on the fix? Here's the error: |
|
Azure Pipelines successfully started running 1 pipeline(s). |
1 similar comment
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Now that all the checks are passing, can we merge this PR? |
If you are a MSFT employee you can view your work branch via this link.
Contribution checklist: